use colcon option for pytest coverage instead of custom pytest-cov options#109
Conversation
Yes but you will need to update the associated colcon extensions as well to make sure they are also compatible with the version you are pinning against. |
|
The CI seems to be failing for a different reason... |
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
330463a to
50ba1e9
Compare
| - run: npm run build | ||
|
|
||
| - uses: ros-tooling/setup-ros@0.0.16 | ||
| - uses: mikaelarguedas/setup-ros@bump-colcon |
There was a problem hiding this comment.
You'll need to revert that now :)
There was a problem hiding this comment.
yup! just wanted to get someone to witness it did pass CI before removing that commit ;)
There was a problem hiding this comment.
Commit removed
50ba1e9 to
a794daf
Compare
|
This has been stale for about a month now, can we get this merged in? It's quite important for being able to get codecov results up. |
|
This is just a suggestion, but you could also use the |
|
Indeed I created the coverage-pytest mixin at the same time as this PR because I was not a fan of the way it was hard-coded in here. I haven't tested recently but IIRC the fact that action-ros-ci hard-codes the pytest-cov parameters was still a problem even though there is a mixin for it. I could update this PR to make use of the mixin by default, but given that this has been months approved and not merged for no apparent reason I'm not convinced it's worth the effort. |
|
@emersonknapp @thomas-moulard is there anything I can do to get this over the line ? |
|
@mikaelarguedas thanks for the reminder, updated the branch, let's merge if the CI is OK. |
|
2 jobs failed. They don't seem related to this change. |
|
@mikaelarguedas Now that #247 is in, are there any changes that need to be made to this? |
|
Not that I know of. I'm not familiar with |
Yep! It was made to work with the usual way(s) |
|
@christophebedard to clarify: All good as in no longer needed or all good as in this is still necessary? :) It seems that |
@piraka9011 sorry! Not sure what you mean by "don't need to specify them here," but this PR is still necessary. We still need to tell |
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> Signed-off-by: Jesse Ikawa <jikawa@amazon.com>
This results in the following pytest-args:
--cov=<PATH/TO/ROS/PACKAGE>--cov-report=html:<PATH/TO/PKG/BUILD/DIR>/coverage.html--cov-report=xml:<PATH/TO/PKG/BUILD/DIR>/coverage.xml--cov-branchhttps://github.com/colcon/colcon-core/blob/af7fa089962a9e9a35926734e17b00cfb4c780a6/colcon_core/task/python/test/pytest.py#L85-L100
The main advantage is that it prevents action-ros-ci to overwrite the output location of the coverage.xml (colcon places it in the build directory of the package) and to have branch coverage in the report.
Note: this needs colcon-core 0.5.1 so doesnt work as is as setup-ros pins colcon version to 0.4.3.
Should I open a separate issue to request a version bump of colcon packages in setup-ros?